fix(apply): key strict-mode baselines on (url, ref), not the source name - #2155

Review round 1 — no issues flagged.

Reviewed 4a576af (all 5 commits, dd835cd..4a576af, against merge-base 9b7970a). The automated review returned no findings, and there are no inline review threads open.

CI is green on that same SHA — all 12 check runs succeeded, including Backend unit tests (the full ~18.5k-test suite under coverage) and Singlebox coverage.

Nothing fixed and nothing disputed this round, so there is no follow-up push and no round 2 will open on its own. The PR is waiting on its human reviewers (@FreddieSun, @xianmuyq); mergeable_state is blocked for that reason and not for a conflict — the base sha matches dev's head exactly.

Two things from the implementation worth a reviewer's eye, both deliberate and both already described in the PR body:

• No compatibility shim for reports written before this change. _last_resolutions skips rows with no url, so strict mode is disarmed for exactly one apply per (url, ref) after deploy, then re-arms itself. This follows the "no compatibility fallback" decision in the task; inferring a repository from an old row's name would invent a pin nobody wrote.
• config_manifest_apply_service.py now sits at exactly 1000 lines, the cap enforced by tests/community/architecture/test_no_oversized_modules.py. It was 996 before this change; commit 5 trimmed the new docstring to fit. The next line added to that file trips the guard, so it is a genuine split candidate — out of scope here.

Generated by Claude Code

Review round 2 — one P1 found and fixed. Pushed 754329d.

Two reviewers ran on 4a576af: the Claude Code review returned no findings, and Codex found a P1 that was real and that I introduced. My round-1 all-clear above stands corrected — it was posted before the Codex review landed, and it was wrong to that extent.

The finding. Keying baselines on (url, ref) alone let one declaration of a repository disarm another's pin. A document may legally declare the same (url, ref) twice — once strict, once non_strict — when some entries may follow a branch and one may not. When the ref moves, the lax declaration delivers the new commit and records it while the pinned one refuses; sharing a baseline, that recorded sha became the pin's baseline, so the next apply with an unchanged document handed the pinned entry the commit it had just rejected. Strict mode degraded to "refuse each move exactly once, then deliver it" — the failure the adopt-after-the-gate ordering exists to prevent, arriving sideways through a second declaration one apply later. I reproduced it with a probe before changing anything.

This was a regression specific to this PR: before it, the two aliases had separate name-keyed baselines and could not interfere.

The fix. mode joins the key, which is now (url, ref, mode), on the rule that a pin may only be advanced by an apply that stood behind it under the same mode. The declarations keep separate histories — the lax one advances and notes its moves, the pinned one refuses until the document re-pins it. SourceResolution.mode rides through as_dict(), the stored-report decode and the response model alongside url, so the lax delivery keeps its report row rather than losing provenance to protect the pin.

This deviates from the task spec, which said baselines are keyed on (url, ref). That spec has the hole above; I judged closing it to be the intent rather than the letter, and flag it here so a human can overrule me. The reviewer's alternative — rejecting conflicting modes for one pair — I passed on because ${BOT_*} substitution happens at apply time, so the check could not live at PUT validation where an author would want it, and it would fail documents that work today. Reasoning in full on the thread.

Every property of the original change survives: re-pinning ref or url still passes, a rename keeps its baseline, a SHA-shaped ref trips neither branch, report rows stay one per declaration.

Validation. 1091 tests pass locally, including the architecture guard. New conformance case test_a_non_strict_alias_cannot_advance_a_strict_pin drives the full two-apply sequence through the front door; I checked it genuinely catches the regression by removing mode=spec.mode from adopt and watching it fail. config_manifest_apply_service.py is back to exactly 1000 lines, still at the cap.

Generated by Claude Code

@codex review

Re-requesting explicitly because a push is not one of your triggers: your P1 on the shared strict/non_strict baseline was fixed in 754329d, which landed after your review of 4a576af. The fix changes the baseline key to (url, ref, mode) and touches source_session.py, source_fetchers.py, config_manifest_apply_service.py, outcomes.py, the report codec and the response model — worth a look at the new head rather than leaving the fix for your own finding unverified.

Generated by Claude Code

Review round 3 — one P1 found and fixed. Pushed 54ff99bc.

Codex re-reviewed 754329d and found that my round-2 fix was incomplete: the same alias hazard survived on the inline road. Round 2's mode-in-the-key change fixed named aliases and left inline ones collapsed.

The finding. adopt de-duplicated on the display alone. A named source's display is its from name, which maps to exactly one (url, ref, mode) — fine. An inline source's display is url@ref, which is mode-blind, so two inline declarations of one repository at one ref but two modes shared it. Whichever resolved first took the slot and the other recorded nothing. When the loser was the strict one it never established a baseline at all — and a pin with no baseline never refuses anything. That is worse than the bug it followed: not "refuse each move once, then deliver it", but a pin disarmed from the first apply onward and permanently.

The fix. adopt de-duplicates on (display, mode). I preferred this to folding the mode into the inline display because the display is user-facing — it is the report row's name and appears in the strict-refusal message — and url@ref is the form the schema doc now promises. Both declarations get a row; they share the name and are distinguished by mode.

The invariant I should have drawn a commit earlier, now written into _recorded's docstring: the recording identity must be at least as fine as the key the recording feeds. Round 2 moved the baseline key to (url, ref, mode) and left the recording identity at display. That is exactly why the named case looked right and the inline case did not, and it is the thing to check the next time either side moves.

Validation. 1085 tests pass locally including the architecture guard; the service file is still at exactly 1000 lines. New conformance case test_two_inline_declarations_at_two_modes_are_two_rows covers both halves — the first-apply recording, and the refusal on the apply after the ref moves. As with the round-2 test, I verified it genuinely catches the regression by collapsing the de-dup key back to a constant mode and watching it fail.

Worth noting for the human reviewers: a push does not re-trigger Codex — its triggers are open-for-review, draft→ready, and an explicit @codex review comment. Round 2's fix would have gone unverified by it had I not asked. I'll keep re-requesting after each push until a round comes back clean from both reviewers.

@codex review

Generated by Claude Code

Review round 4 — one P1 found and fixed. Pushed 25bcb7dc.

Codex found the third instance of a single root cause. Worth stating as one story rather than three, because the pattern matters more than any of the individual bugs:

The finding. @ is legal in a URL path and legal in a refname, so url=".../a@b", ref="c" and url=".../a", ref="b@c" share the display .../a@b@c while having different baseline keys. The second declaration was dropped, and a dropped strict declaration never establishes a baseline, so it accepts every subsequent move.

The fix is Codex's suggested key verbatim: (display, url, ref, mode). display stays because it is not implied by the rest either — two from names over one (url, ref, mode) are two declarations and must stay two rows.

What I got wrong, twice. After round 3 I wrote the invariant into the code: the recording identity must be at least as fine as the key the recording feeds. Then I satisfied it with (display, mode) — a shape that only appears to imply the key. Reasoning about what a display implies is the error; putting the key in the tuple is the fix. Both narrower shapes and the counterexample that killed each are now recorded on _recorded, so the next person sees the counterexamples and not just the conclusion.

Validation. 1086 tests pass locally including the architecture guard; the service file is still at exactly 1000 lines. New conformance case test_an_at_sign_in_a_url_or_ref_does_not_collide_two_declarations covers the collision, the rebuilt baseline map, and the refusal after a move. As with the previous two, I verified it genuinely catches the regression by collapsing the key back to the old shape and watching it fail.

All three P1 threads are resolved or replied to. CI on 25bcb7dc is running.

@codex review

Generated by Claude Code

Review round 5 — one P1 raised, disputed, and resolved as moot. No code change.

Both reviewers have now read 25bcb7dc. The Claude Code review came back Open: 0. Codex raised a fourth P1 — "Preserve baselines from pre-upgrade reports" — and this one I did not fix, which is the first time in this loop I've said no to a Codex finding, so the reasoning is worth stating plainly.

The mechanism it describes is real and I had already documented it: _last_resolutions skips rows lacking url/mode, so a report written before those fields existed contributes no baseline, and a strict source whose ref moved in that window would deliver the moved commit once before re-arming.

It is moot in practice: there is no pre-upgrade data. This feature is still under development and has not shipped, so no stored report predates these fields. The branch is unreachable in any environment that exists, and the upgrade path it guards has no rows to lose a baseline from. Confirmed with the author before deciding.

Neither suggested remedy survives that: reconstructing legacy rows would reconstruct nothing, and failing closed would trade an empty risk for a real regression — a brand-new bot's first apply of a strict source would fail, contradicting test_strict_on_the_first_apply_has_no_opinion.

The null-skip stays for what it actually is: a total function over a nullable field, so a hand-edited or truncated row degrades to "no opinion" rather than keying a baseline on None. Thread resolved with the reasoning on it.

One correction to my own PR body while I'm here: the "already-stored reports contribute no baselines after deployment" paragraph is accurate but reads as though there is a deployed population at risk. There isn't — that sentence exists because the task asked me to state it, not because legacy rows are known to exist.

Running total across the loop: four P1s from Codex, zero from the other reviewer. Three were real and self-inflicted, all one root cause — a recording identity coarser than the key it feeds — and each of my first two fixes was itself too weak. The fourth is this one. Full escalation table in the round-4 comment.

CI on 25bcb7dc: 8/10 green at time of writing; Backend unit tests and Singlebox coverage still in progress. I'll act on either if it fails rather than leaving it.

Generated by Claude Code

CI green on 25bcb7dc — closing out the review loop. All 10 check runs succeeded, including Backend unit tests (16:00:44) and Singlebox coverage (16:05:44), the two that were still running when I posted round 5. mergeable_state is now clean.

Final state: both automated reviewers have read this head, all four review threads are resolved, and @FreddieSun has approved. Nothing outstanding from me.

Summary of the five rounds, for whoever merges this:

The three real findings were a single defect that my first two fixes did not fully close — the escalation table is in the round-4 comment, and the invariant plus both failed shapes are recorded on _recorded in source_session.py so the next person gets the counterexamples rather than the conclusion.

Generated by Claude Code